Adding support for owned types mapped to same table as well as different tables #26706

maumar · 2021-11-16T07:25:28Z

Fix to #26451 - Temporal Table: Owned Entities support
Fix to #26469 - Query: enable temporal tables for table splitting scenarios
Fix to #26705 - Query: model building for temporal table with explicitly defined period columns could fail if conventions are executed in a delayed fashion

Just like any other nav expansion, this only works for AsOf operation.

Added extensibility point to SharedTypeEntityExpandingExpressionVisitor so that we can create temporal table expressions representing owned entities, if their owner is also a temporal table expression.
We also need to match TableExpression information with ITable metadata, for provider specific table-like expressions.

Relaxed validation for table splitting when the table is temporal - it's now allowed as long as all (base type) entities mapped to this table have congruent period and history table mappings.

Column period property is now created explicitly in the temporal table builder rather than relying on conventions to avoid scenario when conventions are delayed and the period property they are supposed to create is not ready by the time we need it.

Fixes #26451
Fixes #26469
Fixes #26705

maumar · 2021-11-16T07:26:47Z

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

                var foreignKey = navigation.ForeignKey;
                if (navigation.IsCollection)
                {
-                    var innerShapedQuery = CreateShapedQueryExpression(
-                        targetEntityType, _sqlExpressionFactory.Select(targetEntityType));
+                    var innerSelectExpression = BuildInnerSelectExpressionForOwnedTypeMappedToDifferentTable(


@smitpatel some refactoring here compared to 6.0 version, which didn't contain the logic for collection (which is also mapped to different table, the logic is pretty much the same) so I DRY'd it a bit

maumar · 2021-11-16T07:29:28Z

src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs

+                }
+
+                // TODO: should we be match case here or not?
+                if (!relationalSharedTypeEntityExpansionHelper.TableMatchesMetadata(unwrappedTable, table))


the code that this is supposed to improve matches case in some cases and not the others - what's the right thing here? Is metadata case sensitive in general? @AndriySvyryd @smitpatel

Yes, we should always match case for strings that we control

maumar · 2021-11-16T07:30:07Z

src/EFCore.SqlServer/Infrastructure/Internal/SqlServerModelValidator.cs

+                {
+                    if (!mappedType.IsTemporal())
+                    {
+                        throw new InvalidOperationException("either none or all types mapped to same table must be temporal");


TODO: add resources

maumar · 2021-11-16T07:31:17Z

src/EFCore.SqlServer/Infrastructure/Internal/SqlServerModelValidator.cs

+                var requiredPeriodStartColumnName = default(string);
+                var requiredPeriodEndColumnName = default(string);
+
+                foreach (var mappedType in mappedTypes.Where(t => t.BaseType == null))


we only look at based types - we already check elsewhere that derived types don't specify temporal information on them

maumar · 2021-11-16T07:32:58Z

src/EFCore.SqlServer/Metadata/Builders/TemporalTableBuilder.cs


            return new TemporalPeriodPropertyBuilder(_entityType, propertyName);
        }

+        private void EnsurePeriodPropertyExists(string propertyName)


TODO: this is probably wrong @AndriySvyryd

We should capture EntityTypeBuilder in TableBuilder and flow it here to use .Property() instead

maumar · 2021-11-16T07:38:04Z

test/EFCore.SqlServer.FunctionalTests/Query/TemporalOwnedQuerySqlServerTest.cs

+                            {
+                                ab.ToTable(tb => tb.IsTemporal(ttb =>
+                                {
+                                    ttb.HasPeriodStart("PeriodStart").HasColumnName("PeriodStart");


this sucks - every owned entity must have the same (redundant) temporal table mapping - we should be smart enough to propagate this information from the owner, but currently its way too much work (as we would need to do it when the new owned type is added as well as when the owner changes) - #15898 is supposed to make it easier

test/EFCore.SqlServer.FunctionalTests/Query/TemporalOwnedQuerySqlServerTest.cs

smitpatel · 2021-11-16T08:28:20Z

src/EFCore.Relational/Query/IRelationalSharedTypeEntityExpansionHelper.cs

+        /// <summary>
+        /// Returns true if the given table expression matches table metadata, false otherwise.
+        /// </summary>
+        public bool TableMatchesMetadata(


The moment we needed to add this, we faulted our own design.

yes, ideally there would be an abstract class for table-like sources (like "tableExpressionBase" and what currently is teb should be named something else?) from which TableExpression and temporalTable expression would inherit. Alternatively TemporalTableExpression should inherit from TableExpression rather than TableExpressionBase, but thats a smell also, since TemporalTableExpression is abstract and TableExpression is not :(

…ent tables Fix to #26451 - Temporal Table: Owned Entities support Fix to #26469 - Query: enable temporal tables for table splitting scenarios Fix to #26705 - Query: model building for temporal table with explicitly defined period columns could fail if conventions are executed in a delayed fashion Just like any other nav expansion, this only works for AsOf operation. Added extensibility point to SharedTypeEntityExpandingExpressionVisitor so that we can create temporal table expressions representing owned entities, if their owner is also a temporal table expression. We also need to match TableExpression information with ITable metadata, for provider specific table-like expressions. Relaxed validation for table splitting when the table is temporal - it's now allowed as long as all (base type) entities mapped to this table have congruent period and history table mappings. Column period property is now created explicitly in the temporal table builder rather than relying on conventions to avoid scenario when conventions are delayed and the period property they are supposed to create is not ready by the time we need it.

AndriySvyryd · 2021-11-16T22:06:04Z

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

@@ -1171,7 +1186,12 @@ protected override Expression VisitExtension(Expression extensionExpression)
                                && navigation.DeclaringEntityType.IsStrictlyDerivedFrom(entityShaperExpression.EntityType));

                        var entityProjection = _selectExpression.GenerateWeakEntityProjectionExpression(
-                            targetEntityType, table, identifyingColumn.Name, identifyingColumn.Table, principalNullable);
+                            targetEntityType,


There are no weak entity types anymore

AndriySvyryd · 2021-11-16T23:16:15Z

src/EFCore.SqlServer/Properties/SqlServerStrings.resx

@@ -325,6 +325,9 @@
  <data name="TemporalSetOperationOnMismatchedSources" xml:space="preserve">
    <value>Set operation can't be applied on entity '{entityType}' because temporal operations on both arguments don't match.</value>
  </data>
+  <data name="TemporalOwnedTypeMappedToDifferentTableOnlySupportedForAsOf" xml:space="preserve">
+    <value>Only '{operationName}' temporal operation is supported for entitiy that owns another entity which is mapped different table.</value>


Suggested change

<value>Only '{operationName}' temporal operation is supported for entitiy that owns another entity which is mapped different table.</value>

<value>Only '{operationName}' temporal operation is supported for entity types that own an entity mapped to a different table.</value>

smitpatel · 2021-11-29T17:50:29Z

src/EFCore.Relational/Metadata/Builders/TableBuilder.cs

        }
+        //[EntityFrameworkInternal]


maumar · 2022-01-13T23:30:29Z

this has been done in 1ca6380 instead

maumar commented Nov 16, 2021

View reviewed changes

test/EFCore.SqlServer.FunctionalTests/Query/TemporalOwnedQuerySqlServerTest.cs Show resolved Hide resolved

maumar requested review from smitpatel and AndriySvyryd November 16, 2021 07:40

smitpatel reviewed Nov 16, 2021

View reviewed changes

maumar force-pushed the fix26469_take3 branch from f919bec to 1e6c4c2 Compare November 16, 2021 08:45

maumar force-pushed the fix26469_take3 branch from 1e6c4c2 to 36dcd84 Compare November 16, 2021 10:28

AndriySvyryd reviewed Nov 16, 2021

View reviewed changes

GfG

3a9f52e

smitpatel reviewed Nov 29, 2021

View reviewed changes

src/EFCore.Relational/Metadata/Builders/TableBuilder.cs

}

//[EntityFrameworkInternal]

Copy link

Contributor

smitpatel Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

maumar closed this Jan 13, 2022

smitpatel deleted the fix26469_take3 branch May 18, 2022 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for owned types mapped to same table as well as different tables #26706

Adding support for owned types mapped to same table as well as different tables #26706

maumar commented Nov 16, 2021

maumar Nov 16, 2021

maumar Nov 16, 2021

AndriySvyryd Nov 16, 2021

maumar Nov 16, 2021

maumar Nov 16, 2021

maumar Nov 16, 2021

AndriySvyryd Nov 16, 2021 •

edited

Loading

maumar Nov 16, 2021

smitpatel Nov 16, 2021

maumar Nov 16, 2021 •

edited

Loading

AndriySvyryd Nov 16, 2021

AndriySvyryd Nov 16, 2021

smitpatel Nov 29, 2021

maumar commented Jan 13, 2022

	<value>Only '{operationName}' temporal operation is supported for entitiy that owns another entity which is mapped different table.</value>
	<value>Only '{operationName}' temporal operation is supported for entity types that own an entity mapped to a different table.</value>

Adding support for owned types mapped to same table as well as different tables #26706

Adding support for owned types mapped to same table as well as different tables #26706

Conversation

maumar commented Nov 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndriySvyryd Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maumar Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maumar commented Jan 13, 2022

AndriySvyryd Nov 16, 2021 •

edited

Loading

maumar Nov 16, 2021 •

edited

Loading